fix: including public channels should check for compliance settings #102
Conversation
fmartingr
added
2: Dev Review
There was a problem hiding this comment.
Some minor comments and we're good to go! 🚀
btw you've also addressed this ticket with your work: https://mattermost.atlassian.net/browse/MM-59309
| @@ -86,10 +82,6 @@ func (kvs Impl) GetLegalHoldByID(id string) (*model.LegalHold, error) { | |||
| } | |||
|
|
|||
| func (kvs Impl) UpdateLegalHold(lh, oldValue model.LegalHold) (*model.LegalHold, error) { | |||
| if err := lh.IsValidForCreate(); err != nil { | |||
There was a problem hiding this comment.
I would feel safer if we also kept the store level validation because we call this method from the job layer as well. We only call CreateLegalHold from the api layer but maybe we should also keep that validation in the store layer to protect ourselves from future modifications?
There was a problem hiding this comment.
You are right that we can reach this from the job layer, let me take a look at it.
There was a problem hiding this comment.
Been checking the code, the job itself only updates one field currently outside of the scope of model.UpdateLegalHold struct, and the code does not follow the same logic path.
Do you think it would add value to add this check here considering we are updating the value atomically?
There was a problem hiding this comment.
I think it just future proofs the code even if it doesn't make a difference right now. We do this in mattermost-server for a lot of our create/update store functions. @esarafianou what is our official stance on this? It's not entirely consistent in our server code either
There was a problem hiding this comment.
Let's create a ticket for this an address for subsequent release. We are in the final stage of releasing this to several customers.
Summary
StartDate < EndDate)Ticket Link
https://mattermost.atlassian.net/browse/MM-59830